Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate all preferences when reading global_prefs and global_prefs_override #5050

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Vulpine05
Copy link
Contributor

@Vulpine05 Vulpine05 commented Jan 1, 2023

Fixes #4916

Description of the Change
Provides a separate function to check values passed from global_prefs and global_prefs_override. Some values were checked in prefs.cpp, but not all. This provides a space in the Client to check all values, and removes the checks in prefs.cpp.

Alternate Designs
My original intent was to keep all checks in prefs.cpp, but as discussed here, it was requested to be in cs_prefs.cpp.

Release Notes
[client] Added validation to all variables from global preferences.

@Vulpine05
Copy link
Contributor Author

Vulpine05 commented Jan 1, 2023

@AenBleidd, @davidpanderson, I need to do a more thorough review of my work, but the overall intent of what I would like to do is here. Could you take a quick look and let me know if you feel I'm on the right track?

Some questions and things I need to complete before I mark this as ready for a complete review:

  • There is one downside to this PR that I see. The global_prefs will be updated by parsing global_prefs.xml, and then it will be updated again by parsing global_prefs_override.xml. I am concerned that the client can read the GLOBAL_PREFS structure either between the two reads, or worse, before an invalid preference is adjusted (a negative percentage, for example). I don't know how likely that is, please let me know if that is a concern. I could always create a second Global_prefs structure, parse data to that structure, and then copy it to the global_prefs that the Client uses.
  • I need to review thoroughly parsing of any date/time fields.
  • I have not added any messages to print to the log/notices if a preference is invalid/adjusted.

@codecov
Copy link

codecov bot commented Jan 1, 2023

Codecov Report

Merging #5050 (a0a6eb7) into master (71e5fd0) will decrease coverage by 0.01%.
Report is 19 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5050      +/-   ##
============================================
- Coverage     10.86%   10.86%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         36073    36094      +21     
  Branches       8339     8328      -11     
============================================
  Hits           3920     3920              
- Misses        31759    31780      +21     
  Partials        394      394              
Files Coverage Δ
lib/prefs.cpp 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

@AenBleidd
Copy link
Member

Hey, @Vulpine05, any plans to continue working on this PR?

@Vulpine05
Copy link
Contributor Author

@AenBleidd, maybe. This is related to my comment here.

@AenBleidd
Copy link
Member

@Vulpine05, from my point of view the behavior should be next:

  • function that reads the preferences from the file (and does actual raw data parsing) should validate that data is technically correct: double is double, integer is integer, etc
  • function (class function to be more precise) that uses the data that was read before should do logical validation, print the error to the user and use the default value instead of logically incorrect one.

In this way we should eliminate the case when the same preferences are checked for the correct logic when read from the different sources.
Also please keep in mind, that sometimes every single source of preferences might contain only the part of them, and thus before starting data validation you should gather them in one place (this is already done, just validation is missed, I suppose).

I hope I've answered your question. If no - don't hesitate to ping me again.
Thank you in advance.

@Vulpine05
Copy link
Contributor Author

@AenBleidd that answers my question, thank you. I think I can wrap this up as I have time. I do have one more noob question. I plan on having the alert message code look like this:
msg_printf(0, MSG_USER_ALERT, "'suspend_cpu_usage' setting is <0. Setting changed to 0.");

Is it okay to hard code in the variable name in as text? The reason I ask is I don't know if that would accidentally be translated with Transifex. It seems easier write the alert message as above than taking the variable name, converting it to text, and then writing the alert message, such as:
msg_printf(0, MSG_USER_ALERT, "'%s' setting is <0. Setting changed to 0.", PrefsVariable);

@AenBleidd
Copy link
Member

@Vulpine05, in order to get the text translated, you need to use the special macro:

_("TEXT TO BE TRANSLATED")

In you particular case I suggest doing like that:

msg_printf(0, MSG_USER_ALERT, "'suspend_cpu_usage' %s <0. %s 0.", _("setting is"), _("Setting changed to"));

@Vulpine05
Copy link
Contributor Author

Got it, thanks for the tip. I think "setting is" and "Setting changed to" are going to be used quite a bit, so I think I'll just create two strings at the beginning of the function and use those.

Add an upper limit for doubles, add day-of-week validation, revises method for printing alerts.
To note where validation occurs in cs_prefs.
@Vulpine05 Vulpine05 marked this pull request as ready for review October 22, 2023 14:04
@Vulpine05 Vulpine05 changed the title [WIP] Validate all preferences when reading global_prefs and global_prefs_override Validate all preferences when reading global_prefs and global_prefs_override Oct 22, 2023
@Vulpine05
Copy link
Contributor Author

@AenBleidd, this PR is ready for review.

@Vulpine05
Copy link
Contributor Author

Some screen captures of the messages that could appear. Note that under the notices tab some are not appearing, but they do show up in the event log. When there are one or two validation alerts, this doesn't appear to be a problem. I suspect this may be an unrelated issue.
Notices tab

Event Log

@davidpanderson
Copy link
Contributor

Text shown to users shouldn't include variable names like 'suspend_cpu_usage'.
It should use names from the prefs UI.

These names are pretty much consistent between the web prefs interface
and the Manager's interface.
However, many of them are different in Android.
I suggest that we fix this in Android, and then return to this PR.

@CharlieFenton
Copy link
Contributor

Are some of these checks not also made in the preferences dialog? If so, they should also be done there and the dialog should refuse to be saved until they are corrected.

It does make sense to also do the checks here when reading from the prefs files, since the user could manually edit them.

@Vulpine05
Copy link
Contributor Author

Text shown to users shouldn't include variable names like 'suspend_cpu_usage'.
It should use names from the prefs UI.

These names are pretty much consistent between the web prefs interface
and the Manager's interface.
However, many of them are different in Android.
I suggest that we fix this in Android, and then return to this PR.

The intent is to alert the user that their is a mistake in the global_prefs(_override) file. Although the preference can be fixed via the Manager, I decided to list the preference name from the xml so it could be easily referenced there. I think this isn't too different when the Manager notifies the user of an error in a app_config file for a project.

Additionally, I am not sure how to easily describe when variable to fix via the Manager without being verbose.

@Vulpine05
Copy link
Contributor Author

Are some of these checks not also made in the preferences dialog?
This already happens in the Manager but that is independent of this PR. This PR validates variables in the Client.

For example, if you accidentally type -1 for suspend when computer is in use in global_prefs, then work will be suspended indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Provide limit checks when reading global_prefs_override
4 participants